Sync with Fabric Loader 0.16.2 #448
Conversation
This reverts commit 09b6423
* Added support for 1.21.2 in McVersionLookup * Update minecraft/src/main/java/net/fabricmc/loader/impl/game/minecraft/McVersionLookup.java Co-authored-by: modmuss <modmuss50@gmail.com> --------- Co-authored-by: modmuss <modmuss50@gmail.com> (cherry picked from commit 991d38d8a62e89f00aa3038b52f428f95b7d44bc)
|
Unfortunately the reason why we haven't updated to FLoader 0.16.x is we're encountering new issues with mixin - for example the first mixin in my tests fails with this error: Caused by: java.lang.ClassFormatError: Duplicate method name "callIsNamespaceValid" with signature "(Ljava.lang.String;)Z" in class file org/quiltmc/qsl/resource/loader/mixin/IdentifierAccessor
at java.base/java.lang.ClassLoader.defineClass1(Native Method)
at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
at org.quiltmc.loader.impl.launch.knot.KnotClassLoader.defineClassFwd(KnotClassLoader.java:3crash-2024-08-17_05.05.06.8224-quilt_loader.txt Inspecting the exported mixin file shows that the methods and annotation is duplicated: $ javap -p -s -v IdentifierAccessor.class
Classfile /<omitted>/QuiltLoaderTests/runs/20240817_050427/1.18.2-client/.mixin.out/class/org/quiltmc/qsl/resource/loader/mixin/IdentifierAccessor.class
Last modified 17 Aug 2024; size 862 bytes
SHA-256 checksum 6c78310c1ed3621d8e5a26a779837e6db37f6b525a48d5ffddca9a44e864799a
Compiled from "IdentifierAccessor.java"
public interface org.quiltmc.qsl.resource.loader.mixin.IdentifierAccessor
minor version: 0
major version: 61
flags: (0x0601) ACC_PUBLIC, ACC_INTERFACE, ACC_ABSTRACT
this_class: #2 // org/quiltmc/qsl/resource/loader/mixin/IdentifierAccessor
super_class: #4 // java/lang/Object
interfaces: 0, fields: 0, methods: 2, attributes: 2
Constant pool:
#1 = Utf8 org/quiltmc/qsl/resource/loader/mixin/IdentifierAccessor
<snip>
#27 = Utf8 RuntimeInvisibleAnnotations
{
public static boolean callIsNamespaceValid(java.lang.String);
descriptor: (Ljava/lang/String;)Z
flags: (0x1009) ACC_PUBLIC, ACC_STATIC, ACC_SYNTHETIC
Code:
stack=1, locals=1, args_size=1
0: aload_0
1: invokestatic #20 // Method net/minecraft/class_2960.callIsNamespaceValid$quilt_resource_loader_$md$b666f8$0:(Ljava/lang/String;)Z
4: ireturn
LocalVariableTable:
Start Length Slot Name Signature
0 0 0 namespace Ljava/lang/String;
RuntimeVisibleAnnotations:
0: #12()
org.spongepowered.asm.mixin.gen.Invoker
1: #13(#14=s#15)
org.spongepowered.asm.mixin.transformer.meta.MixinProxy(
sessionId="bffba2d2-646f-4ad9-9cca-c3cce8b666f8"
)
MethodParameters:
Name Flags
namespace
public static boolean callIsNamespaceValid(java.lang.String);
descriptor: (Ljava/lang/String;)Z
flags: (0x1009) ACC_PUBLIC, ACC_STATIC, ACC_SYNTHETIC
Code:
stack=1, locals=1, args_size=1
0: aload_0
1: invokestatic #20 // Method net/minecraft/class_2960.callIsNamespaceValid$quilt_resource_loader_$md$b666f8$0:(Ljava/lang/String;)Z
4: ireturn
LocalVariableTable:
Start Length Slot Name Signature
0 0 0 namespace Ljava/lang/String;
RuntimeVisibleAnnotations:
0: #12()
org.spongepowered.asm.mixin.gen.Invoker
1: #13(#14=s#15)
org.spongepowered.asm.mixin.transformer.meta.MixinProxy(
sessionId="bffba2d2-646f-4ad9-9cca-c3cce8b666f8"
)
MethodParameters:
Name Flags
namespace
}
SourceFile: "IdentifierAccessor.java"
RuntimeInvisibleAnnotations:
0: #6(#7=[c#8])
org.spongepowered.asm.mixin.Mixin(
value=[class Lnet/minecraft/class_2960;]
)
1: #6(#7=[c#8])
org.spongepowered.asm.mixin.Mixin(
value=[class Lnet/minecraft/class_2960;]
) |
FLoader 0.16.2 is still a mess? damn, and i thought y'all were allergic to updates; |
reader.accept(node, 0);
reader.accept(node, readerFlags);erm, I think that might be it lol. I haven't finished testing it yet though :p |
|
It turns out it's our fault. Or at least it's our fault until someone else who encountered mixin problems initially can reproduce them. Thanks for opening this PR! I wouldn't have taken a proper look into this if you didn't open this :D |
|
...and I spoke too soon. Sorry :( Caused by: org.spongepowered.asm.mixin.injection.throwables.InjectionError: LVT in net/minecraft/class_3218::method_8413(Lnet/minecraft/class_2338;Lnet/minecraft/class_2680;Lnet/minecraft/class_2680;I)V has incompatible changes at opcode 59 in callback #lithium:lithium.mixins.json:entity.inactive_navigations.ServerWorldMixin from mod lithium->@Inject::updateActiveListeners(Lnet/minecraft/class_2338;Lnet/minecraft/class_2680;Lnet/minecraft/class_2680;ILorg/spongepowered/asm/mixin/injection/callback/CallbackInfo;Lnet/minecraft/class_265;Lnet/minecraft/class_265;Ljava/util/List;)V. |
hm, are you sure this doesn't have anything to do with mixin compatibility shenanigans? (also, something tells me that quilt should perhaps adopt a "0.27.0+ shall be mixin 0.14.0" thingy instead of being always latest) |
|
Possibly, in this case lithium requires fabric-loader 0.12.11, so uses 0.9.2 mixin compatibility. |
ok yeah, this is wrong; if you have an explicit dep on floader 0.12.0-, you have 0.10.0 mixin compat mode; this explains why the errors are all reminiscent of mixin 0.9.2 shenanigans this will need a massive refactoring though, so that quilt mods are also accounted into this system (they do still exist! kinda!), so yeah, maybe this is no longer my job and now it's yours? :p |
|
So loader's mixin compatibility test code looks like this: for (LoaderMixinVersionEntry version : versions) {
if (minLoaderVersion.compareTo(version.loaderVersion) >= 0) { // lower bound is >= current version
Log.debug(LogCategory.MIXIN, "Mod %s requires loader version %s, using mixin compatibility %s", metadata.id(), minLoaderVersion, version.mixinVersion);
return version.mixinVersion;
} else {
Log.debug(LogCategory.MIXIN, "Mod %s requires loader version %s, using 0.9.2 mixin compatability", metadata.id(), minLoaderVersion);
return FabricUtil.COMPATIBILITY_0_9_2;
}
}facepalm. Well, that was an easy fix...
Yeah, that's a good point! All quilt mods currently get the latest mixin version, no compat code applied :/ |
This should work as a fix for now
How so? |
I'm being a little dramatic (after all, I can't think of a better option), but I think it's very surprising behavior that the minimum version set in the mod metadata influences mixin. Nobody would reasonably expect local capture (iirc, i'm a little rusty) to change behavior based on if they had put a |
|
Seems fairly reasonable to me that changing your min version would opt you into new behaviours. |
|
I'd definitely prefer it if you could change the mixin compatibility level separately to the loader version - since they are two separate libraries. But not in this pull request, since it feels somewhat separate from updating to floader 1.16.2 |
|
a. changing the dep doesn't opt you into any other behaviours, it's just for mixin (afaik) |
|
Yeah, I feel like a pin to 0.10.0 will be good enough to let us think about another solution for now; but maybe we could have the QLoader min version be a fallback of some sorts (but also have it be explicitly printed on the logs) in favor of explicit mixin compat on maybe the QMJ? The template mod is always here to help with good practices |
How is it not? For fabric, is this even documented anywhere outside the code or logging? When I saw this in floader's code I was quite surprised. |
Isn't that what the |
|
Folks, can we have a separate issue created please? I'd rather have the FLoader 0.16.2 compatibility merged asap than having to worry about Quilt mods' mixin compat shenanigans and stall the compat even further; we can have the compat as a 0.27.0-beta.1, then we can figure out a good solution for Quilt later; the thing is: we took way too long to be compatible with that update in the name of "yes Fabric will fix their stuff" when we were actually broken all along, so can we please not bikeshed this PR? thank you |
Manually merged to not loose history. Also includes a changelog, and new loader beta version
please?
(This update uses cherry picks (and a big revert of a revert) to sync with FLoader 0.16.2 (a merge was going to ruin everything), also it implements the FLoader entrypoint change to QLoader in order to avoid some torture)